Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

doc: fix exec stdout & stderr default type in child_process #7361

Closed
wants to merge 3 commits into from
Closed

doc: fix exec stdout & stderr default type in child_process #7361

wants to merge 3 commits into from

Conversation

sartrey
Copy link
Contributor

@sartrey sartrey commented Jun 22, 2016 •
edited
Loading

Checklist
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)
Description of change

add words in child_process docs about invalid Buffer encoding leading to a default buffer output

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Jun 22, 2016
@sartrey sartrey changed the title fix doc about child_process exec & execFile doc: fix exec stdout & stderr default type in child_process Jun 22, 2016
@sartrey
Copy link
Contributor Author

sartrey commented Jun 22, 2016

#6666

@mscdex mscdex mentioned this pull request Jun 22, 2016
Merged
cjihrig
cjihrig reviewed Jun 22, 2016
View reviewed changes
doc/api/child_process.md
@@ -164,7 +164,7 @@ The `stdout` and `stderr` arguments passed to the callback will contain the
stdout and stderr output of the child process. By default, Node.js will decode
the output as UTF-8 and pass strings to the callback. The `encoding` option
can be used to specify the character encoding used to decode the stdout and
stderr output. If `encoding` is `'buffer'`, `Buffer` objects will be passed to
stderr output. If `encoding` is `'buffer'` (or not a valid Buffer encoding), `Buffer` objects will be passed to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should say something like:

If encoding is 'buffer', or an unrecognized character encoding, Buffer objects will be passed to

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please wrap lines at 80 characters.

@sartrey
Copy link
Contributor Author

sartrey commented Jun 23, 2016

@cjihrig I will edit these lines.

cjihrig
cjihrig reviewed Jun 23, 2016
View reviewed changes
doc/api/child_process.md
@@ -164,8 +164,8 @@ The `stdout` and `stderr` arguments passed to the callback will contain the
stdout and stderr output of the child process. By default, Node.js will decode
the output as UTF-8 and pass strings to the callback. The `encoding` option
can be used to specify the character encoding used to decode the stdout and
stderr output. If `encoding` is `'buffer'`, `Buffer` objects will be passed to
the callback instead.
stderr output. If `encoding` is `'buffer'`, or an unrecognized character encoding,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a quick check, it seems like these lines are still > 80 characters.

@sartrey
Copy link
Contributor Author

sartrey commented Jun 24, 2016

@cjihrig
Yeah, these lines are still > 80 characters.
However, I found there are some long lines before my change.
I try to find balance between line wrap & easier to read.
Please tell me if I should keep wrap at accurate 80 characters.

Sorry for my poor English, here is my true meaning in Chinese

我在文档的前半部分看到一些行也超过了80字符，
看上去可能是要保持一行意义完整。
所以，我也尝试在一行里取得某种平衡。
改动后的行大约80字符左右，可能有一点超过。

如果我必须精确遵循80字符，请告诉我，我会修改的。

@cjihrig
Copy link
Contributor

cjihrig commented Jun 24, 2016

Please wrap them at 80 characters or less. There are some other longer lines, but they shouldn't be that way (no need to change the other ones in this PR though).

@sartrey
Copy link
Contributor Author

sartrey commented Jun 25, 2016

@cjihrig Please review this new commit.

@cjihrig
Copy link
Contributor

cjihrig commented Jun 27, 2016

LGTM

cjihrig pushed a commit that referenced this pull request Jun 27, 2016
@sartrey @cjihrig
doc: clarify child_process stdout/stderr types
926707f 
Clarify how the encoding option interacts with the data
type of child process stdout and stderr.

Fixes: #6666
PR-URL: #7361
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig
Copy link
Contributor

cjihrig commented Jun 27, 2016

Thanks! Landed in 926707f.

@cjihrig cjihrig closed this Jun 27, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
@sartrey @Fishrock123
doc: clarify child_process stdout/stderr types
44bc638 
Clarify how the encoding option interacts with the data
type of child process stdout and stderr.

Fixes: #6666
PR-URL: #7361
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
Merged
@MylesBorins MylesBorins mentioned this pull request Jul 11, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
@sartrey
doc: clarify child_process stdout/stderr types
101f7be 
Clarify how the encoding option interacts with the data
type of child process stdout and stderr.

Fixes: #6666
PR-URL: #7361
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@sartrey @MylesBorins
doc: clarify child_process stdout/stderr types
3ce574e 
Clarify how the encoding option interacts with the data
type of child process stdout and stderr.

Fixes: #6666
PR-URL: #7361
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@sartrey
doc: clarify child_process stdout/stderr types
b17a58c 
Clarify how the encoding option interacts with the data
type of child process stdout and stderr.

Fixes: #6666
PR-URL: #7361
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@sartrey
doc: clarify child_process stdout/stderr types
71c68e0 
Clarify how the encoding option interacts with the data
type of child process stdout and stderr.

Fixes: #6666
PR-URL: #7361
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@sartrey
doc: clarify child_process stdout/stderr types
6f5d939 
Clarify how the encoding option interacts with the data
type of child process stdout and stderr.

Fixes: #6666
PR-URL: #7361
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@sartrey
doc: clarify child_process stdout/stderr types
35ee35c 
Clarify how the encoding option interacts with the data
type of child process stdout and stderr.

Fixes: #6666
PR-URL: #7361
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sartrey @cjihrig @MylesBorins @nodejs-github-bot